Skip to content

Implemented a generic filters plugin #1634

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 16 commits into
base: master
Choose a base branch
from
Draft

Implemented a generic filters plugin #1634

wants to merge 16 commits into from

Conversation

ankurbodhe
Copy link

This PR migrates and enhances the chained_filter_controller originally implemented during the ROSCon 2024 control workshop. The controller enables chaining and filtering of state interfaces through a sequence of filters.

Changes introduced:

  1. Ported chained_filter_controller from workshop reference implementation.
  2. Developed unit tests covering:
    • Initialization and lifecycle behavior.
    • Filter configuration and chaining.
    • Correct propagation of state values through the filter stack.

@ankurbodhe ankurbodhe mentioned this pull request Apr 9, 2025
Open
4 tasks
@ankurbodhe
Copy link
Author

@christophfroehlich I just made another commit to fix the issues in the workflow.. requesting you to approve to workflow

@saikishor
Copy link
Member

@ankurbodhe can you fix the pre-commit jobs?

Is this ready for review?

christophfroehlich
christophfroehlich requested changes Apr 19, 2025
View reviewed changes
Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for contribution!
In general, have a look at other packages in this repository to follow the (code) style, copyright claims etc.

Please address my comments and also fix the pre-commit failures as Sai already has mentioned.

chained_filter_controller/test/test_chained_filter.cpp
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test if a filter is loaded and used to process data.

@christophfroehlich
Copy link
Contributor

@ankurbodhe can I help you to finish this PR? We also have to apply the changes of #1697 to the new CMakeLists.

@ankurbodhe
Copy link
Author

@christophfroehlich Sorry had a hectic work schedule... my calendar just freed up and would be working on completing the PR.. will be working on this everyday now until completion.

@christophfroehlich
Copy link
Contributor

no worries. don't hesitate to ask if there are any doubts how to proceed

@ankurbodhe ankurbodhe force-pushed the abodhe-impl-filters-plugin branch from c61a23d to c3a79de Compare May 30, 2025 23:40
@christophfroehlich
Copy link
Contributor

@ankurbodhe please don't force-push to already reviewed PRs, it just makes subsequent reviews slow. There is no need for a clean history, as we squash PRs at the time of merging into the base branches.
Ping me again if it is ready for the next review round.

@github-actions GitHub Actions
Copy link
Contributor

This PR is stale because it has been open for 45 days with no activity. Please tag a maintainer for help on completing this PR, or close it if you think it has become obsolete.

@github-actions github-actions bot added the stale label Jul 16, 2025
@github-actions github-actions bot removed the stale label Jul 22, 2025
@codecov Codecov
Copy link

codecov bot commented Jul 22, 2025

Codecov Report

Attention: Patch coverage is 70.73171% with 24 lines in your changes missing coverage. Please review.

Project coverage is 85.57%. Comparing base (76c07f9) to head (030abca).

Files with missing lines Patch % Lines
chained_filter_controller/src/chained_filter.cpp 46.34% 21 Missing and 1 partial ⚠️
...ned_filter_controller/test/test_chained_filter.cpp 93.10% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1634      +/-   ##
==========================================
- Coverage   85.66%   85.57%   -0.10%     
==========================================
  Files         123      126       +3     
  Lines       12408    12489      +81     
  Branches     1056     1058       +2     
==========================================
+ Hits        10629    10687      +58     
- Misses       1430     1453      +23     
  Partials      349      349
Flag Coverage Δ
unittests 85.57% <70.73%> (-0.10%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...oller/test/test_load_chained_filter_controller.cpp 100.00% <100.00%> (ø)
...ned_filter_controller/test/test_chained_filter.cpp 93.10% <93.10%> (ø)
chained_filter_controller/src/chained_filter.cpp 46.34% <46.34%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christophfroehlich christophfroehlich christophfroehlich requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ankurbodhe @saikishor @christophfroehlich